Skip to content

UW-255: Changes needed for SRW fill_jinja_template replacement.#237

Merged
christinaholtNOAA merged 2 commits into
developfrom
srw_integration_changes
May 12, 2023
Merged

UW-255: Changes needed for SRW fill_jinja_template replacement.#237
christinaholtNOAA merged 2 commits into
developfrom
srw_integration_changes

Conversation

@christinaholtNOAA
Copy link
Copy Markdown
Collaborator

Description

While integrating uwtools into SRW, a few changes were needed.

  • The arguments when generating the templates should point to default values, so I removed those.
  • The name of the called script is now included in the logged information since it's helpful in the output in the log for SRW.

In addition, running the tests on Hera highlighted that we don't yet have jsonschema installed/supported there, so we should skip that test when the package isn't available.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

I ran the linter and pytests manually on hera.

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes need updates to the documentation. I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • New and existing tests pass with my changes
  • Any dependent changes have been merged and published

Copy link
Copy Markdown
Contributor

@WeirAE WeirAE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests clear, good change, just checking one thing - is the log entry in SRW not capturing the script name because of our log name change or is the SRW script not capturing the child call? We may want to change other logging to ensure the entries are still showing the correct script

Comment thread src/uwtools/j2template.py
@christinaholtNOAA
Copy link
Copy Markdown
Collaborator Author

@WeirAE SRW doesn't really have consistent logging throughout. There is a logger at the top level of the generate workflow script, but it isn't named or passed to any of the calls to other functions. There are no classes currently, so we're in a bit of a gray area.

In this way, we can have the output that is sent to either a basic bash log, or a python log show up with some additional information. For example, this function is called separate three times for three separate config files from the bash run script for the forecast model.

Copy link
Copy Markdown
Contributor

@WeirAE WeirAE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fine for now, we can revisit taking over logging for other apps later. LGTM

Copy link
Copy Markdown
Contributor

@venitahagerty venitahagerty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@christinaholtNOAA
Copy link
Copy Markdown
Collaborator Author

@venitahagerty Did I address your concerns? Could I do more?

@venitahagerty
Copy link
Copy Markdown
Contributor

I'm good

@christinaholtNOAA christinaholtNOAA merged commit e1b3b6f into develop May 12, 2023
@christinaholtNOAA christinaholtNOAA deleted the srw_integration_changes branch May 12, 2023 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants